Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New lint: [self_named_constructor] #7403

Merged
merged 2 commits into from
Jul 19, 2021
Merged

New lint: [self_named_constructor] #7403

merged 2 commits into from
Jul 19, 2021

Conversation

Anthuang
Copy link
Contributor

@Anthuang Anthuang commented Jun 26, 2021 •
edited by Manishearth
Loading

Adds the self_named_constructor lint for detecting when an implemented method has the same name as the type it is implemented for.

changelog: [self_named_constructor]

closes: #7142

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 26, 2021
@camsteffen
Copy link
Contributor

Lint name idea: self_named_constructor. It should not lint impl Foo { fn bar() -> Bar; }.

@Anthuang
Copy link
Contributor Author

Lint name idea: self_named_constructor. It should not lint impl Foo { fn bar() -> Bar; }.

That's a better name, I will update the diff.

@bors
Copy link
Contributor

bors commented Jun 30, 2021

☔ The latest upstream changes (presumably #7400) made this pull request unmergeable. Please resolve the merge conflicts.

popzxc
popzxc reviewed Jun 30, 2021
View reviewed changes
tests/ui/self_named_constructor.stderr Outdated Show resolved Hide resolved
clippy_lints/src/self_named_constructor.rs Outdated
impl_item.span,
&format!("constructor `{}` has the same name as the type", impl_item.ident.name),
|diag| {
diag.span_help(impl_item.span, &format!("consider renaming `{}()` to `new()`", impl_item.ident.name));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message will be bogus if the new method is already present in the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got rid of it for now, since it felt like the action needed to resolve the lint was pretty self explanatory. Let me know if it is better to leave a message, and to check for the existence of new first.

clippy_lints/src/self_named_constructor.rs Outdated Show resolved Hide resolved
tests/ui/self_named_constructor.rs Outdated Show resolved Hide resolved
popzxc
popzxc approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@popzxc popzxc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from a couple of nits. Great job 👍

clippy_lints/src/default.rs Outdated Show resolved Hide resolved
tests/ui/missing_const_for_fn/could_be_const.rs Outdated Show resolved Hide resolved
@popzxc
Copy link
Contributor

popzxc commented Jul 4, 2021

@Manishearth, what will you say?

@bors
Copy link
Contributor

bors commented Jul 5, 2021

☔ The latest upstream changes (presumably #7243) made this pull request unmergeable. Please resolve the merge conflicts.

@Manishearth Manishearth changed the title New lint: [redundant_method_names] New lint: [self_named_constructor] Jul 18, 2021
@Manishearth
Copy link
Member

@bors r+

sorry about the delay!

@bors
Copy link
Contributor

bors commented Jul 18, 2021

📌 Commit 357a8f0 has been approved by Manishearth

bors added a commit that referenced this pull request Jul 18, 2021
@bors
Auto merge of #7403 - Anthuang:redundant-method-names, r=Manishearth
e8fb6a9 
New lint: [`self_named_constructor`]

Adds the `self_named_constructor` lint for detecting when an implemented method has the same name as the type it is implemented for.

changelog: [`self_named_constructor`]

closes: #7142
@bors
Copy link
Contributor

bors commented Jul 18, 2021

⌛ Testing commit 357a8f0 with merge e8fb6a9...

@bors
Copy link
Contributor

bors commented Jul 18, 2021

💔 Test failed - checks-action_test

@Anthuang
Copy link
Contributor Author

No worries! Just ran bless again, that should fix the test failure

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 19, 2021

📌 Commit e9e10d2 has been approved by Manishearth

@bors
Copy link
Contributor

bors commented Jul 19, 2021

⌛ Testing commit e9e10d2 with merge f70a074...

@bors
Copy link
Contributor

bors commented Jul 19, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing f70a074 to master...

@bors bors merged commit f70a074 into rust-lang:master Jul 19, 2021
@bors bors mentioned this pull request Jul 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@popzxc popzxc popzxc approved these changes

@Manishearth Manishearth Manishearth requested changes

Assignees

@Manishearth Manishearth

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warn against Type::type methods
6 participants
@Anthuang @rust-highfive @camsteffen @bors @popzxc @Manishearth